Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Java] Define step definitions and hooks with minimal ceremony #2415

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

[Java] Define step definitions and hooks with minimal ceremony #2415

wants to merge 7 commits into from

Conversation

rozenshteyn
Copy link

Is your pull request related to a problem? Please describe.

[Java] Allow non-private classes to contain hooks and allow hook annotations in non-public methods (#2370)

Describe the solution you have implemented

  1. When scanning methods of a class, allow the class to have package-private and protected visibility in addition to the previously allowed public visibility.
  2. When scanning methods of a class, allow the hook-annotated methods to have package-private and protected visibility in addition to the previously allowed public visibility.

Additional context
None.

@mpkorstanje
Copy link
Contributor

Wow. I'll have to find some time to review this. It is likely that I won't have any in the next two weeks.

@codecov
Copy link

codecov bot commented Nov 7, 2021 •
edited
Loading

Codecov Report

Merging #2415 (91f48b2) into main (04281f2) will decrease coverage by 0.04%.
The diff coverage is 64.28%.

❗ Current head 91f48b2 differs from pull request most recent head 0158029. Consider uploading reports for the commit 0158029 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main    #2415      +/-   ##
============================================
- Coverage     83.64%   83.60%   -0.05%     
+ Complexity     2677     2641      -36     
============================================
  Files           319      317       -2     
  Lines          9441     9325     -116     
  Branches        918      908      -10     
============================================
- Hits           7897     7796     -101     
+ Misses         1204     1195       -9     
+ Partials        340      334       -6
Impacted Files Coverage Δ
.../src/main/java/io/cucumber/java/MethodScanner.java 81.25% <64.28%> (-1.11%) ⬇️
.../io/cucumber/java/JavaDocStringTypeDefinition.java 90.90% <0.00%> (-9.10%) ⬇️
.../src/main/java/io/cucumber/guice/GuiceFactory.java 90.90% <0.00%> (-1.95%) ⬇️
...latform/engine/CucumberEngineExecutionContext.java 85.36% <0.00%> (-1.31%) ⬇️
...main/java/io/cucumber/core/options/CurlOption.java 94.23% <0.00%> (-0.65%) ⬇️
...java/io/cucumber/core/plugin/PublishFormatter.java 0.00% <0.00%> (ø)
...o/cucumber/core/stepexpression/StepExpression.java 100.00% <0.00%> (ø)
...a/io/cucumber/docstring/DocStringTypeRegistry.java 100.00% <0.00%> (ø)
...ucumber/core/stepexpression/DocStringArgument.java 85.71% <0.00%> (ø)
...tring/DocStringTypeRegistryDocStringConverter.java 100.00% <0.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04281f2...0158029. Read the comment docs.

@mpkorstanje mpkorstanje added this to the v8.0.0 milestone Nov 28, 2021
mpkorstanje
mpkorstanje approved these changes Dec 1, 2021
View reviewed changes
Copy link
Contributor

@mpkorstanje mpkorstanje left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick aside. I don't see any immediate problems with this.

I think this can be merged with the next major version.

However prior to merging we should update the code in the examples module, the java module documentation and the archetype so it is clear that Cucumber with minimal ceremony is preferred.

Given that it will be some time before the next major version rolls around, and given that those changes mentioned above are fragile I think it would be prudent to delay them until merging.

After merging the https://github.com/cucumber/cucumber-java-skeleton should also be updated.

java/src/test/java/io/cucumber/java/MethodScannerTest.java
MethodScanner.scan(BaseSteps.class, backend);
assertThat(scanResult, contains(new SimpleEntry<>(method, method.getAnnotations()[0])));
assertThat(scanResult,
contains(new SimpleEntry<>(publicMethod, publicMethod.getAnnotations()[0]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using org.hamcrest.collection.IsMapContaining.hasEntry for all these.

Would have been better if I had done that the first time round, but better late then never.

@mpkorstanje mpkorstanje changed the title [Java] Allow non-private classes to contain hooks and allow hook annotations in non-public methods (#2370) [Java] Allow step definitions and hooks to be defined with minimal ceremony Dec 1, 2021
@mpkorstanje mpkorstanje changed the title [Java] Allow step definitions and hooks to be defined with minimal ceremony [Java] Define step definitions and hooks to be defined with minimal ceremony Dec 1, 2021
@mpkorstanje mpkorstanje changed the title [Java] Define step definitions and hooks to be defined with minimal ceremony [Java] Define step definitions and hooks with minimal ceremony Dec 1, 2021
@mpkorstanje
Copy link
Contributor

Note to self: Drop 0158029 before merging. Looks like it was an attempted fix for a different issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpkorstanje mpkorstanje mpkorstanje approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v8.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@rozenshteyn @mpkorstanje